Review generated by Claude (Anthropic's coding assistant), posted at the repo owner's request.

Thanks for tackling this — universal F1 for rules is a genuine accessibility win. Claude tested the PR end-to-end (real BackgammonGame wired to a real DocumentManager, real {"type":"keybind","key":"f1"} packet) and the happy path works, including locale fallback (French user with French translation gets French; Spanish user falls back to English) and correct exclusion of non-public locales. Spectators can also use F1, which is good. A few things to address before merge.

Blocking

1. Silent / gibberish failure when _table._server is missing. _action_show_rules calls user.speak_l("action-not-available"), but action-not-available does not exist in server/locales/en/main.ftl. Fluent throws KeyError, the localizer falls through to f"[{message_id}]", and the screen-reader user literally hears "[action-not-available]". Per CLAUDE.md ("silence is a bug"), this isn't acceptable. Add the Fluent message, or reuse an existing one.

2. Dead hasattr guard hides wiring problems. In menu_management_mixin.py:241:

if hasattr(self, "_table") and self._table and hasattr(self._table, "_server") and self._table._server:

self._table is always defined on Game (set to None in __post_init__), so hasattr(self, "_table") is dead code. The triple-guard turns "table not yet wired to server" into the silent failure above. If wiring is broken we want a loud error, not a vague "action not available."

3. Reaches into private API and reinvents existing logic. The handler calls doc_manager._get_visible_locale_codes(...) (underscore-prefixed) and rolls its own locale-selection loop. DocumentManager._select_display_title_locale already does exactly this and is what get_documents_in_category uses. The new loop also has dead code — or locale_code == title_locale never fires, because title_locale is only set right before break.

Design / cleanup

4. The new _is_always_enabled callback is redundant. Two functionally identical callbacks already live in action_visibility_mixin.py:

def _is_leave_game_enabled(self, player):     # line 113
    return None
def _is_show_actions_enabled(self, player):   # line 161
    return None

Both are wired to KeybindState.ALWAYS actions (ctrl+q, escape) — either could have been used for show_rules. A generic _is_always_enabled reads better at the call site than reusing _is_leave_game_enabled for an unrelated action, but then the right move is to migrate leave_game and show_actions over to the new helper and delete the two redundant copies. As written, the PR leaves three "return None" callbacks sitting side-by-side, which is the worst of both worlds.

(For clarity, this is an is_enabled callback on Action — not a new KeybindState enum value. The PR correctly uses the existing KeybindState.ALWAYS.)

5. Tight coupling. self._table._server._documents reaches three layers down. A single accessor on Table (e.g., table.get_documents()) would localize this knowledge.

6. Trailing whitespace on many lines in _action_show_rules.

7. Possible duplicate F1 packet on the desktop client. F1 was already mapped in _map_function_key (line 704) and sent via on_char_hook when focus is on the menu list. The PR adds it as a global accelerator too. wxWidgets accelerators usually consume the event before EVT_CHAR_HOOK, but the order is platform-sensitive — worth manually verifying on Windows that pressing F1 with focus on the menu list doesn't fire two keybind packets. Net effect would be harmless (idempotent editbox open), but a duplicate is a smell.

8. F1 from the lobby (no game) is silent. Client always sends the packet when connected; server drops it if the user isn't at a table. Not a regression, but for a "global rules shortcut" some feedback ("you must be at a table to view rules") would be friendlier.

Testing

The PR description says "Deployed a local test environment" and "Traced inbound network packets," but the PR adds zero automated tests for new code that touches keybind dispatch, document lookup, and locale selection. Claude wrote 8 targeted tests covering the end-to-end path, locale fallback, private-locale exclusion, missing-document handling, and the _table._server is None case (which is what surfaced the missing-Fluent-ID bug). ~150 LOC available if you'd like them attached or pushed as a follow-up commit.

Done.
Summary of fixes:

1. Fixed Silent Failure (KeyError):
   Replaced an unlocalized action-not-available string with the existing, localized action-locked string. This prevents a Fluent KeyError that was causing screen-reader users to hear literal brackets when an action failed.

2. Removed Dead Guards:
   Removed unnecessary hasattr triple-guards (self._table and self._table._server). This ensures that if a game's table wiring is broken during development, it fails loudly with an AttributeError instead of hiding the problem behind a generic "action not available" message.

3. Reused DocumentManager APIs:
   Refactored the custom, manual (and slightly broken) locale-selection loop. The rules handler now correctly delegates to DocumentManager's native _select_display_title_locale and _select_visible_title methods to resolve the best localization, matching the rest of the codebase.

4. Deduplicated Action Callbacks:
   Deleted the redundant _is_leave_game_enabled and _is_show_actions_enabled callbacks (which simply returned None). Both of these actions were migrated to use the newly introduced, generic _is_always_enabled helper.

5. Reduced Tight Coupling:
   Added a get_documents() accessor to the Table class. This abstracts away the server's internal layout, allowing the game rules handler to avoid reaching three layers deep (self._table._server._documents).

6. Cleaned Up Formatting:
   Removed all trailing whitespace across the _action_show_rules method block.

7. Added Lobby Feedback:
   Updated the server's global keybind handler to intercept F1 when a user isn't at a table. Instead of silently dropping the packet, it now gives the friendly, localized feedback: "You must be at a table to view rules."

8. Fixed Desktop Client Duplicate Packet Smell:
   Removed F1 from the manual _map_function_key handler in the desktop client. Because the PR correctly registered F1 as a global application accelerator, leaving it in the manual hook map could cause wxWidgets to fire both events on Windows when the menu list had focus. Relying solely on the global accelerator guarantees exactly one network packet is sent.

Review by Claude (Opus 4.7), driven by the PR author's collaborator. Posted on their behalf.

What works

• Client: F1 promoted from focus-bound to a global wx.AcceleratorEntry. Fires from anywhere in the desktop app, sends {"type":"keybind","key":"f1"} to server.
• Server: show_rules action + f1 keybind (KeybindState.ALWAYS, include_spectators=True) added to the standard action set; routes to _action_show_rules which pulls rules markdown via Table.get_documents() → DocumentManager.
• Off-table fallback: server speaks action-must-be-at-table when F1 fires with no table.
• Refactor: consolidates _is_show_actions_enabled and _is_leave_game_enabled into a single _is_always_enabled. Clean, no dangling references.
• Action visibility wiring: is_hidden=_is_always_hidden + show_in_actions_menu=True is correct (hidden from turn menu, visible in Esc actions menu). Worth a code comment so future readers don't "fix" the apparent contradiction.

Test results

• 151/151 existing tests pass across the touched mixins and games (test_event_handling_mixin, test_game_utils_mixins, test_active_tables_menu, test_table_core, test_yahtzee, test_backgammon).
• CLI simulate backgammon --bots 2: game completes; rules appear in spectator's actions menu as "Show rules (F1)".
• Hand-driven smoke test (proper initialize_lobby flow): F1 in lobby ✓, F1 in active game ✓, spectator F1 ✓, missing rules folder speaks documents-no-content ✓, locale fallback to English when user locale missing ✓, private locales skipped ✓.

Issues

1. CLI keybind validation regresses (concrete bug)

$ uv run cli.py simulate backgammon --bots 2 --validate-keybinds
...
Keybind validation: FAILED
  - Keybind 'f1' smoke test crashed: 'NoneType' object has no attribute 'get_documents'

The CLI's smoke test runs each keybind synthetically without setting game._table, so self._table.get_documents() crashes. Per CLAUDE.md, CLI tooling regressions take priority. One-line fix: nullcheck self._table and either early-return or speak action-must-be-at-table.

2. Table.get_documents() and _action_show_rules reach into private state

def get_documents(self) -> Any:
    return self._server._documents

No nullcheck on _server. Worse, _action_show_rules calls three private methods on DocumentManager:

• doc_manager._get_visible_locale_codes(...)
• doc_manager._select_display_title_locale(...)
• doc_manager._select_visible_title(...)

These are underscore-prefixed for a reason. If DocumentManager's internals change, every game's F1 silently breaks. Better: expose a public DocumentManager.get_localized_document(folder, viewer_locale, *, include_private=False) -> (title, content) | None that encapsulates locale selection, and have _action_show_rules call only that.

3. The handler doesn't belong in MenuManagementMixin

_action_show_rules is a 50-line, document-system-specific handler. MenuManagementMixin is meant for menu/status_box plumbing. Either pull it onto Game, or — better — give it its own rules_action_mixin.py so it's discoverable.

4. Hardcoded English string

In action_set_creation_mixin.py:

self.define_keybind(
    "f1",
    "Show rules",   # ← hardcoded English, bypasses Fluent
    ["show_rules"],
    ...
)

The action label uses Localization.get(locale, "show-rules") ✓, but the keybind name passed to define_keybind is a literal English string. Per CLAUDE.md: "Every announcement must go through Fluent… No hardcoded English strings reaching players." Please replace with a Fluent key. (Note: pre-existing keybinds in the codebase have the same pattern — that's a separate cleanup, but the new one shouldn't ship that way.)

5. No tests added

97 lines added, 0 tests. CLAUDE.md testing priority (item #3 in the priority list). The PR's testing notes describe manual smoke testing, not regression coverage.

Recommendation

Minimum to ship:

1. Nullcheck self._table in _action_show_rules (fixes CLI smoke test failure).
2. Localize the "Show rules" keybind name through Fluent.
3. Add at least four core tests (lobby F1, active F1, spectator F1, missing folder).

Bigger improvements (worth doing now while the API is fresh):
4. Replace the three private DocumentManager._* calls with a single public method.
5. Move _action_show_rules out of MenuManagementMixin.

I won't be updating this PR from now on.
See my comment on #243.

The code isn't perfect, but Claude is just being Claude.

I'm hoping someone comes in and submits new PRs that implement those features or a maintainer steps-in and updates the thing.